-
Notifications
You must be signed in to change notification settings - Fork 2
Issue 40 docker image for releases #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b2c7c86 to
dc97fcc
Compare
|
Still have to figure out nextgenhealthcare/connect-docker#42 |
echo $VMOPTIONS | sed -E '
# Remap doubled commas to \x01
s/,,/\x01/g;
# Break into lines by unescaped commas
s/,/\n/g;
# Restore the escaped commas
s/\x01/,/g;
' >> "$APP_DIR/oieserver.vmoptions" |
Since we now have full control over the code, it doesn't have to follow the old pattern. There is, of course, desirability in having the most backwards compatibility. I'm even thinking of defining a new optional env to read values with a different delimiter, |
Fair. Not sure how fancy this needs to get. File/config mounts can easily be used to replace custom.vmoptions in docker and k8s. I don't think that's as much an option for mirth.properties since it doesn't seem to support includes. Perhaps adding includes to mirth.properties is an option? That would also simplify the install/upgrade story. Edit: |
|
I tested these builds by running the compose locally. It works. OIE launches and I can log in. The README was largely dervied from NG, it says how to actually USE the software. I also grabbed the examples directory and updated it to our image |
Good point! I shall Investigate this a little. |
|
Actually, question for @OpenIntegrationEngine/maintainers, should we break compat by dropping the |
|
@mgaffigan regarding the Given # mirth.properties
key1 = "value1"
key2 = "value2"
include = more.properties# more.properties
key2 = "value2 but some more"
key3 = "value3" |
328f6fa
NO. Using includes and vmoptions is a good idea, but this should be a separate PR in a later issue so we can consider the compat issues. |
I don't think we should break it either. Keeping the And I would probably keep the VMOPTIONS appending to the main |
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com>
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com> Signed-off-by: Jon Bartels <jon.bartels@teladochealth.com>
Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com>
753d5d2 to
23589e3
Compare
| ARG UID=14285 | ||
| ARG GID=14285 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected behavior is that UID and GID are 1000 so that bind mount permissions work for the majority of users where their host UID is also 1000.
See nextgenhealthcare/connect-docker#49. This caused quite a few issues in the NextGen slack when the ubuntu image started including an ubuntu user with UID 1000 in their base images, so NextGen bumped the mirth UID to 1001.
Here is where they corrected it by renaming the ubuntu user to mirth so that the mirth user retained UID 1000. nextgenhealthcare/connect-docker@233502c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other piece of that is even if you aren't using bind mounts, and you try to use existing volumes from a nextgen container, but the nextgen container had a different UID from the new oie container, the oie user won't have appropriate access to the existing volume.
Referencing OpenIntegrationEngine/engine#40 and taking inspiration from OpenIntegrationEngine/engine#126.
The PR creates the Dockerfiles required for official runtime Docker images.